Jackson 3.x GeoJson (de-)serializers #5102
Conversation
Adds a GeoJsonJackson3Module. Closes #5100
| * | ||
| * @author Jens Schauder | ||
| */ | ||
| public class GeoJsonJackson3Configuration implements SpringDataJackson3Modules { |
There was a problem hiding this comment.
Given the opportunity I think we should align the naming our newly added GeoJson Jackson3 support to follow what is outlined in spring-projects/spring-data-redis#3219
There was a problem hiding this comment.
GeoJsonJacksonConfiguration is Jackson 2, so is GeoJacksonModule
Should those be renamed to ...Jackson2...
To be honest I find that awfully confusing. Especially, but not only, since commons also goes with Jackson3
There was a problem hiding this comment.
I'd leave the ones we have as is and name the others something like JacksonGeoJson... maybe.
There was a problem hiding this comment.
I really don't like that. If anyone looks at this in a year (or myself in two weeks), how are they supposed to tell what the reason is behind having JacksonGeoJsonConfiguration and GeoJsonJacksonConfiguration?
How about this: We go with Jackson3 for now and once we remove the Jackson 2 support, we rename everything to just Jackson? This way the distinction is clear as long we have both and once we have only one we get clean names (until Jackson 4 of course).
There was a problem hiding this comment.
jackson does not contain the 3 in any of it, still it's the new baseline and 2 the old one, so we also could have ripped com.fasterxmll out and go with tools.jackson without changing. the leading jackson is more of an alignment with other projects
|
|
||
| public class GeoJsonJackson3Module { | ||
|
|
||
| private static Version version = new Version(3, 2, 0, null, "org.springframework.data", |
There was a problem hiding this comment.
version should be updated
There was a problem hiding this comment.
Darn, I wanted to ask about that.
How exactly ist this version used?
Should we also update the version of the Jackson 2 Module?
Should we we keep it in sync with the module version?
There was a problem hiding this comment.
IIRC the version is just some info that can be obtained for info/logging but not used otherwise. I'd sync it with the version we initially release it with or when changes are made to it.
| @ContextConfiguration | ||
| public class GeoJsonConfigurationIntegrationTests { | ||
| public class GeoJsonJackson3ConfigurationIntegrationTests { | ||
|
|
||
| @Configuration | ||
| @EnableSpringDataWebSupport | ||
| static class Config {} | ||
|
|
||
| @Autowired GeoJsonModule geoJsonModule; | ||
| @Autowired | ||
| GeoJsonJackson3Module geoJsonModule; | ||
|
|
||
| @Test // DATAMONGO-1181 | ||
| @Test // GH-5100 | ||
| public void picksUpGeoJsonModuleConfigurationByDefault() { | ||
| assertThat(geoJsonModule).isNotNull(); | ||
| } |
There was a problem hiding this comment.
Please do not remove the the wiring of the old module since it is still present and should be picked up in case Jackson2 is present. SpringDataWebConfigurationImportSelector will pick and choose based on what's present.
It should still be possible to wire the old one.
There was a problem hiding this comment.
I don't think I removed the wiring for the old version.
I just limited the test to the new version, because testing the old version now requires fiddling with the class path, which I considered too much effort for something that is deprecated.
But I guess I can put it back in.
...va/org/springframework/data/mongodb/config/GeoJsonJackson3ConfigurationIntegrationTests.java
Outdated
Show resolved
Hide resolved
|
@schauder mind naming (initial) branches for existing issues following the established pattern without adding any extra |
Since I have a hard time remembering pure numbers I find the suffix extremely helpful. |
Adds a GeoJsonJackson3Module.
Closes #5100